conflicts: add labels to conflicts #7692
Conversation
scott2000
force-pushed
the
scott2000/conflict-labels
branch
5 times, most recently
from
68c536e to
df41bd5
Compare
|
Note that this PR seems to implement it a bit differently than what I suggested on #1176 . My suggestion there was to record a label associated with each diff in the conflict, not with each term. I don't know which is better, and I haven't reviewed the code yet (only at a high enough level to notice that). One advantage of attaching a label to a diff is that it can also make sense when the involved trees don't come directly from commits. |
scott2000
force-pushed
the
scott2000/conflict-labels
branch
from
df41bd5 to
861033d
Compare
Oh yeah, I may have misunderstood. I think I was implementing it based on a Discord conversation we had a few weeks ago. I think it may actually be better to record labels per term rather than per diff because it seems to me like it would work better with the current simplification logic. Sometimes simplifying conflicts causes two diffs to be combined into a single diff, and it might be difficult to combine the labels when this happens. For instance, if you rebase a commit with parent A onto commit B, it might get a conflict label indicating "diff from A to B". If you then rebase it from commit B to commit C, I think ideally the label should indicate "diff from A to C". With term labels, this happens automatically, since the two terms with label "B" are discarded when simplifying. With diff labels, I think we'd either need some non-trivial logic to merge labels, or we'd need to settle for something like "diff from A to B, then diff from B to C".
I think we can still get this to work with term labels. For instance, I'm thinking we could do something like this for conflicts resulting from interactive squash: It might still be tricky though for cases where |
scott2000
force-pushed
the
scott2000/conflict-labels
branch
7 times, most recently
from
6efdc57 to
1d695b8
Compare
scott2000
changed the base branch from
main
to
scott2000/remove-legacy-merged-tree
scott2000
force-pushed
the
scott2000/conflict-labels
branch
2 times, most recently
from
529abe8 to
1ae55b4
Compare
scott2000
force-pushed
the
scott2000/remove-legacy-merged-tree
branch
from
c09ae21 to
f41d655
Compare
scott2000
force-pushed
the
scott2000/conflict-labels
branch
2 times, most recently
from
7133792 to
c06fac3
Compare
scott2000
force-pushed
the
scott2000/remove-legacy-merged-tree
branch
from
f41d655 to
6408973
Compare
scott2000
force-pushed
the
scott2000/conflict-labels
branch
6 times, most recently
from
51c1184 to
87d3a8b
Compare
scott2000
force-pushed
the
scott2000/conflict-labels
branch
5 times, most recently
from
45a0bef to
b4d526d
Compare
Adding this as a separate type will help maintain the invariant that resolved merges cannot have labels. I added `Arc` since the labels will often need to be cloned, such as in `MergedTree::sub_tree`, `MergedTree::id`, and when reading and writing root trees. I think that storing separate conflict labels for each term of the conflict is the best approach for a couple reasons. Mainly, I think it integrates well with the existing conflict algebra. For instance, a diff of (A - B) and a diff of (B - C) can be easily combined to create a new diff of (A - C), and if we associate a label with each term, then the labels will also naturally be carried over as well. Also, I think it would be simpler to implement than other approaches (such as storing labels for diffs instead of terms), since conflict labels can re-use existing logic from `Merge<T>`. For simplicity, I also think we shouldn't allow mixing labeled terms and unlabeled terms (i.e. if any term doesn't have a label, then we discard all labels and leave the entire merge unlabeled). I think it could be confusing to have conflicts where, for instance, one side says "rebase destination" and another side only says "side #2" with no further information. In cases like these, I think it's better to just fall back to the old labels. In the future, I expect that most conflicts should have labels (since we should eventually be adding labels everywhere conflicts can happen).
Since two merged trees can now have the same contents but different conflict labels, many places that previously compared `MergedTreeId` for equality now need to ignore the conflict labels. To solve this, I added a `MergedTreeId::has_changes` method to check whether the underlying `Merge<TreeId>` has any changes.
To implement simplification of conflict labels, I decided to add more functions such as `zip` and `unzip` to `Merge`. I think these functions could be useful in other situations so I thought this was a nice solution, but an alternative solution could be to make `get_simplified_mapping` and `apply_simplified_mapping` public and manually apply the same mapping to both merges.
The old method is renamed to `MergedTree::merge_unlabeled` to make it easy to find unmigrated callers. The goal is that almost all callers will eventually use `MergedTree::merge` to add labels, unless the resulting tree is never visible to the user.
Conflict labels are stored in a separate header for backwards compatibility.
If a conflicted file didn't change, but the conflict labels changed, we will still need to re-materialize the file in the working copy to ensure that the conflict labels are displayed properly.
I swapped the order of added and removed term in the label for diff sections, because I think having the added term always come first makes conflicts easier to read, especially when there are conflict labels. For instance, if I see this conflict: ``` <<<<<<< Conflict 1 of 1 %%%%%%% Changes from parents of ysrnknol 7a20f389 to rebase destination (rtsqusxu 2768b0b9) -base +left +++++++ Contents of rebased commit (ysrnknol 7a20f389) right >>>>>>> Conflict 1 of 1 ends ``` It's not obvious that the first side is the changes from the rebase destination, whereas this is more clear to me: ``` <<<<<<< conflict 1 of 1 %%%%%%% rebase destination (rtsqusxu 2768b0b9) compared with parents of ysrnknol 7a20f389 -base +left +++++++ rebased commit (ysrnknol 7a20f389) right >>>>>>> conflict 1 of 1 ends ```
An example with parents `rtsqusxu` and `ysrnknol`: ``` <<<<<<< conflict 1 of 1 %%%%%%% rtsqusxu 2768b0b9 compared with vpxusssl 38d49363 -base +left +++++++ ysrnknol 7a20f389 right >>>>>>> conflict 1 of 1 ends ```
An example of rebasing `rtsqusxu` onto `ysrnknol`: ``` <<<<<<< conflict 1 of 1 %%%%%%% rebase destination (rtsqusxu 2768b0b9) compared with parents of ysrnknol 7a20f389 -base +left +++++++ rebased commit (ysrnknol 7a20f389) right >>>>>>> conflict 1 of 1 ends ```
An example of squashing `ysrnknol` into `rtsqusxu`: ``` <<<<<<< conflict 1 of 1 +++++++ squash destination (rtsqusxu 2768b0b9) updated in destination %%%%%%% squashed commit (ysrnknol 7a20f389) compared with parents of ysrnknol 7a20f389 -base +squashed >>>>>>> conflict 1 of 1 ends ```
scott2000
force-pushed
the
scott2000/conflict-labels
branch
from
b4d526d to
b40a321
Compare
|
Also I suggest that if bookmarks are used they show optionally, names are more descriptive than numbers. |
Yeah, that would definitely be nice. I'm thinking it would be a follow-up to this PR since it would require some additional changes to pass down the required information. |
Resolves #1176. I'm not sure if this is the best solution, but I wanted to try it out and see how it feels. This could probably be split into smaller PRs if we decide we want to follow this approach. These are the steps I took:
MergedTreeId::Legacyvariant, since it complicated implementation and I believe it is unnecessary now.MergedTreeandMergedTreeId(as suggested by @martinvonz)Many other places still do not add labels (e.g.
jj split,jj absorb), so they would have to be added later.Example conflict markers for merge with
rtsqusxuandysrnknolas parents:Example conflict markers for rebase of
ysrnknolontortsqusxu:Example conflict markers for squash of
ysrnknolintortsqusxu:Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)